-
Notifications
You must be signed in to change notification settings - Fork 562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
advisor: use json for package database #4999
Conversation
…dvisor.Package everyhwere)
advisor/backend.go
Outdated
return err | ||
} | ||
} | ||
sil = append(sil, Package{Snap: snapName, Version: version}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No summary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I should add a comment. For the mapping of command->snap we do not need the summary, nothing is using that. But that is confusing so a comment is needed here.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
"bar": {"bar"}, | ||
"meh": {"foo", "bar"}, | ||
c.Check(dump, DeepEquals, map[string]string{ | ||
"foo": `[{"name":"foo","version":"1.0"}]`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/name/snap/
to match the json tags for Package struct
Tests are not unit-happy:
EDIT: and I see Maciej found the same thing. |
1dc9678
to
3e72368
Compare
Codecov Report
@@ Coverage Diff @@
## master #4999 +/- ##
==========================================
+ Coverage 79.11% 79.12% +0.01%
==========================================
Files 478 478
Lines 35490 35507 +17
==========================================
+ Hits 28078 28095 +17
+ Misses 5191 5183 -8
- Partials 2221 2229 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR also simplifies the code a bit by using advisor.Package to be stored in json instead of a helper "snapInfo" struct.
Based on #4845